Skip to content

Control.flushQueueOnDnd: simplify commit #2573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Control.flushQueueOnDnd: simplify commit #2573

wants to merge 1 commit into from
+2 −2

Conversation

@tmssngr
Copy link
Contributor

@tmssngr tmssngr commented Oct 1, 2025

Inlined sendOrPost for this special case making it more obvious that an (immediate) sendEvent is happening.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025 •
edited
Loading

Test Results

  114 files   -  4    114 suites   - 4   11m 33s ⏱️ - 3m 46s
4 607 tests  - 44  4 588 ✅  - 46  19 💤 +2  0 ❌ ±0 
  311 runs   - 19    308 ✅  - 18   3 💤  - 1  0 ❌ ±0 

Results for commit ccae0e8. ± Comparison against base commit a9598b8.

This pull request removes 44 tests. 
AllGTKTests Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
AllGTKTests Test_GtkConverter ‑ test_HeuristicASCII_emptyString
AllGTKTests Test_GtkConverter ‑ test_HeuristicASCII_letterA
AllGTKTests Test_GtkConverter ‑ test_HeuristicASCII_letters
AllGTKTests Test_GtkConverter ‑ test_HeuristicUTF16LE_null
AllGTKTests Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
AllGTKTests Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
AllGTKTests Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
AllGTKTests Test_GtkConverter ‑ test_HeuristicUTF16_letter
AllGTKTests Test_GtkConverter ‑ test_HeuristicUTF16_letters
…
This pull request skips 4 tests. 
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_getContentsBothClipboards
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_Clipboard ‑ test_setContentsBothClipboards
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_dnd_FileTransfer ‑ test_nativeToJava
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Shell ‑ test_activateEventSend

♻️ This comment has been updated with latest results.

akurtakov
akurtakov reviewed Oct 2, 2025
View reviewed changes
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Control.java
// This is to preserve backwards Cocoa/Win32 compatibility.
Event mouseDownEvent = dragDetectionQueue.getFirst();
mouseDownEvent.data = Boolean.valueOf(true); // force send MouseDown to avoid subsequent MouseMove before MouseDown.
mouseDownEvent.data = null;
Copy link
Member

@akurtakov akurtakov Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting data to null is not needed too. Please remove it, it might have the positive side effect of not losing any other data that could have been there.

Copy link
Contributor Author

@tmssngr tmssngr Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is taken from sendOrPost.

Copy link
Contributor

@merks merks Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, formerly it was setting true into data so that this method would do a send and this method does also set data to null:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Control.java

Lines 5128 to 5140 in 0e04bc9

private boolean sendOrPost(int type, Event event) {
assert event.data != null : "event.data should have been a Boolean, but received null";
boolean send = (Boolean) event.data;
event.data = null;
if (send) {
sendEvent (type, event);
if (isDisposed ()) return false;
} else {
postEvent (type, event);
}
return event.doit;
}

But I don't think there is a compelling reason to set data to null to maybe replace some other value. If there is another value in data (in the future), replacing it with null is like to be unhelpful.

Copy link
Contributor Author

@tmssngr tmssngr Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You propose to change event.data while I want to keep it null as is. Maybe some application code relies on having event.data set null?

@tmssngr tmssngr force-pushed the Control.flushQueueOnDnd-simplification branch 2 times, most recently from abe2183 to ccae0e8 Compare November 5, 2025 09:59
Control.flushQueueOnDnd: simplify commit
ccae0e8 
sendOrPost set event.data to null before sending, so keep that behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merks merks merks left review comments

@akurtakov akurtakov akurtakov left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tmssngr @merks @akurtakov